Skip to content

[SPARK-25867][ML] Remove KMeans computeCost#22875

Closed
mgaido91 wants to merge 7 commits intoapache:masterfrom
mgaido91:SPARK-25867
Closed

[SPARK-25867][ML] Remove KMeans computeCost#22875
mgaido91 wants to merge 7 commits intoapache:masterfrom
mgaido91:SPARK-25867

Conversation

@mgaido91
Copy link
Contributor

@mgaido91 mgaido91 commented Oct 29, 2018

What changes were proposed in this pull request?

The PR removes the deprecated method computeCost of KMeans.

How was this patch tested?

NA

@SparkQA
Copy link

SparkQA commented Oct 29, 2018

Test build #98201 has finished for PR 22875 at commit 9ee93b1.

  • This patch fails build dependency tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Oct 29, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Oct 29, 2018

Test build #98210 has finished for PR 22875 at commit 9ee93b1.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Oct 29, 2018

Test build #98212 has finished for PR 22875 at commit 62f8606.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 5, 2018

cc @holdenk @srowen

@SparkQA
Copy link

SparkQA commented Nov 10, 2018

Test build #98680 has finished for PR 22875 at commit c2fdddc.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Nov 15, 2018

Test build #98874 has finished for PR 22875 at commit 6e2d501.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 20, 2018

any commetn on this @srowen @holdenk ?

@SparkQA
Copy link

SparkQA commented Nov 20, 2018

Test build #99048 has finished for PR 22875 at commit f09e985.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 20, 2018

retest this please

srowen
srowen requested changes Nov 20, 2018
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just grepping for computeCost -- do any examples need updating? I see some instances but they may be a different computeCost.

But what about BisectingKMeans too?

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 20, 2018

Yes, the problem is that there is also the computeCost of BisectingKMeans. I proposed to deprecate it in 2.4 and remove in 3.0, but I didn't manage to have it done for 2.4 (please refer to the discussions on #22764 and #22756 for the details). So computeCost of BisectingKMeans cannot be removed in 3.0 (unfortunately). The examples for KMeans have already been updated using ClusteringEvaluator in #19676.

@srowen
Copy link
Member

srowen commented Nov 20, 2018

How about BisecingKMeansModel? That's what I was looking at. That computeCost is also deprecated.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 20, 2018

Yes, it was deprecated in #22756 and it is deprecated since 3.0, so we cannot remove it...

@srowen
Copy link
Member

srowen commented Nov 20, 2018

Ah OK I wasn't reading that carefully.

@SparkQA
Copy link

SparkQA commented Nov 20, 2018

Test build #99057 has finished for PR 22875 at commit f09e985.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen approved these changes Nov 21, 2018
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the merge conflict now, but looks OK otherwise.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 21, 2018

thanks for your review @srowen

@SparkQA
Copy link

SparkQA commented Nov 21, 2018

Test build #99109 has finished for PR 22875 at commit 2b18e55.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 21, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Nov 21, 2018

Test build #99123 has finished for PR 22875 at commit 2b18e55.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 21, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Nov 21, 2018

Test build #99133 has finished for PR 22875 at commit 2b18e55.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

srowen
srowen approved these changes Nov 21, 2018
View reviewed changes
Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK by me; apologies for another rebase though. We're merging lots of changes that need MiMa exclusions and it keeps conflicting.

@mgaido91
Copy link
Contributor Author

mgaido91 commented Nov 22, 2018

sure, thanks @srowen , no need to apologize at all, thanks for your help reviewing this.

@SparkQA
Copy link

SparkQA commented Nov 22, 2018

Test build #99174 has finished for PR 22875 at commit 692ff63.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • class RuleSummary(
  • class QueryPlanningTracker
  • class QueryExecution(

@srowen
Copy link
Member

srowen commented Nov 22, 2018

Merged to master

@asfgit asfgit closed this in dd8c179 Nov 22, 2018
jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@mgaido91
[SPARK-25867][ML] Remove KMeans computeCost
5973b9a 
## What changes were proposed in this pull request?

The PR removes the deprecated method `computeCost` of `KMeans`.

## How was this patch tested?

NA

Closes apache#22875 from mgaido91/SPARK-25867.

Authored-by: Marco Gaido <marcogaido91@gmail.com>
Signed-off-by: Sean Owen <sean.owen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@srowen srowen srowen approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mgaido91 @SparkQA @srowen

Comments